xds/bootstrap: support new fields for xds federation#4867
xds/bootstrap: support new fields for xds federation#4867menghanl wants to merge 6 commits intogrpc:masterfrom
Conversation
| // | ||
| // The token "%s", if present in this string, will be replaced with the IP | ||
| // and port on which the server is listening. (e.g., "0.0.0.0:8080", | ||
| // "[::]:8080"). For example, a value of // "example/resource/%s" could |
There was a problem hiding this comment.
Get rid of // inside this comment line?
| // If not present in the bootstrap file, defaults to | ||
| // "xdstp://<authority_name>/envoy.config.listener.v3.Listener/%s". | ||
| ClientListenerResourceNameTemplate string | ||
| // XDSServer is FIXME. |
There was a problem hiding this comment.
What does this comment mean?
There was a problem hiding this comment.
FIXME is supposed to remind me to fix it before sending.
Updated.
| return fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) | ||
| } | ||
| case "client_listener_resource_name_template": | ||
| // FIXME: must start with "xdstp://<authority_name>" |
There was a problem hiding this comment.
Why can't we just validate it in this PR? url.Parse() should be able to do it.
There was a problem hiding this comment.
It's done. line 393.
I forgot to delete the comment.
| // Post-process the authorities' client listener resource template field: | ||
| // - if set, it must start with "xdstp://<authority_name>/" | ||
| // - if not set, it defaults to "xdstp://<authority_name>/envoy.config.listener.v3.Listener/%s" | ||
| for n, a := range config.Authorities { |
There was a problem hiding this comment.
I was searching for a while as to where the authority_name is fetched from. Could we name n and a to something more meaningful. I understand these have a very small scope, but n and a don't convey much at all.
xds/internal/xdsclient/client.go
Outdated
| } | ||
|
|
||
| switch config.TransportAPI { | ||
| switch config.XDSServer.TransportAPI { |
There was a problem hiding this comment.
Do we need this check going forward? If so, should we have a similar check for the server configs per authority?
menghanl
force-pushed
the
federation_bootstrap
branch
from
7d53614 to
f3117fd
Compare
…ration Support new bootstrap fields - client_default_listener_resource_name_template - authorities Changes - new struct for server config (URI, creds, v2/v3, node), and reused for top level and per-authority server - and fix all the usages - node proto message is now for each server, to be consistent with TransportAPI version - config test to use cmp.Diff
menghanl
force-pushed
the
federation_bootstrap
branch
from
f3117fd to
964bc20
Compare
|
Replaced by #4892 |
2 participants
Support new bootstrap fields
Changes
RELEASE NOTES: